release/21.x: [KeyInstr] Fix verifier check (#149043) #149053
Conversation
|
@SLTozer What do you think about merging this PR to the release branch? |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: None (llvmbot) ChangesBackport 653872f Requested by: @OCHyams Full diff: https://github.com/llvm/llvm-project/pull/149053.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index f97c7b6445984..0dde045453e3a 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -170,6 +170,10 @@ void CGDebugInfo::addInstToSpecificSourceAtom(llvm::Instruction *KeyInstruction,
if (!Group || !CGM.getCodeGenOpts().DebugKeyInstructions)
return;
+ llvm::DISubprogram *SP = KeyInstruction->getFunction()->getSubprogram();
+ if (!SP || !SP->getKeyInstructionsEnabled())
+ return;
+
addInstSourceAtomMetadata(KeyInstruction, Group, /*Rank=*/1);
llvm::Instruction *BackupI =
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 8004077b92665..9afbefb8c08ef 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -3185,12 +3185,6 @@ void Verifier::visitFunction(const Function &F) {
CheckDI(SP->describes(&F),
"!dbg attachment points at wrong subprogram for function", N, &F,
&I, DL, Scope, SP);
-
- if (DL->getAtomGroup())
- CheckDI(DL->getScope()->getSubprogram()->getKeyInstructionsEnabled(),
- "DbgLoc uses atomGroup but DISubprogram doesn't have Key "
- "Instructions enabled",
- DL, DL->getScope()->getSubprogram());
};
for (auto &BB : F)
for (auto &I : BB) {
@@ -5492,6 +5486,15 @@ void Verifier::visitInstruction(Instruction &I) {
if (MDNode *N = I.getDebugLoc().getAsMDNode()) {
CheckDI(isa<DILocation>(N), "invalid !dbg metadata attachment", &I, N);
visitMDNode(*N, AreDebugLocsAllowed::Yes);
+
+ if (auto *DL = dyn_cast<DILocation>(N)) {
+ if (DL->getAtomGroup()) {
+ CheckDI(DL->getScope()->getSubprogram()->getKeyInstructionsEnabled(),
+ "DbgLoc uses atomGroup but DISubprogram doesn't have Key "
+ "Instructions enabled",
+ DL, DL->getScope()->getSubprogram());
+ }
+ }
}
if (auto *DII = dyn_cast<DbgVariableIntrinsic>(&I)) {
diff --git a/llvm/test/DebugInfo/KeyInstructions/Generic/verify.ll b/llvm/test/DebugInfo/KeyInstructions/Generic/verify.ll
index 0f8f505c51a58..5d73b2669ccda 100644
--- a/llvm/test/DebugInfo/KeyInstructions/Generic/verify.ll
+++ b/llvm/test/DebugInfo/KeyInstructions/Generic/verify.ll
@@ -7,6 +7,8 @@
define dso_local void @f() !dbg !10 {
entry:
+; Include non-key location to check verifier is checking the whole function.
+ %0 = add i32 0, 0, !dbg !14
ret void, !dbg !13
}
@@ -20,3 +22,4 @@ entry:
!11 = !DISubroutineType(types: !12)
!12 = !{null}
!13 = !DILocation(line: 1, column: 11, scope: !10, atomGroup: 1, atomRank: 1)
+!14 = !DILocation(line: 1, column: 11, scope: !10)
|
|
Once the original commit has completely cleared CI, LGTM. |
|
The original commit seems to have failed a bunch of runners. Can we check if that needs to be fixed before a backport? |
Just had a look through the 11 fails - as far as I can tell none of them are related to this change. There's still two pending builds ( Happy to wait longer or wait for another pair of eyes, whatever you think is best! |
|
As for the pre-merge on this one: the abi-compare bot thing seems cool, though I don't think the reported failure is for this patch, I've not touched any function signatures here |
Yeah it's not correct until we made the RC1 release. If this branch is rebased it will pass. But I think it's fine enough to merge this at this point. |
The verifier check was in the wrong place, meaning it wasn't actually checking many instructions. Fixing that causes a test failure (coro-dwarf-key-instrs.cpp) because coros turn off the feature but still annotate instructions with the metadata (which is a supported situation, but the verifier doesn't like it, and it's hard to teach the verifier to like it). Fix that by avoiding emitting any key instruction metadata if the DISubprogram has opted out of key instructions. (cherry picked from commit 653872f)
github-project-automation
bot
moved this from Needs Merge
to Done
in LLVM Release Status
|
@OCHyams (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
Backport 653872f
Requested by: @OCHyams